Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed PHP 8.4 deprecations #185

Open
wants to merge 2 commits into
base: 2.0.x
Choose a base branch
from

Conversation

davidbyoung
Copy link

Fixes #184

@davidbyoung davidbyoung changed the base branch from master to 2.0.x September 23, 2024 12:26
@davidbyoung
Copy link
Author

@erusev Is this repo still being maintained?

@erusev
Copy link
Owner

erusev commented Sep 29, 2024

Were you able to run the tests?

@davidbyoung
Copy link
Author

davidbyoung commented Sep 29, 2024

I did run the unit tests on PHP 8.4RC1 on my local machine, and they all passed. It's a backward-compatible change and that just makes the nullable parameters explicitly nullable.

@davidbyoung
Copy link
Author

btw, I noticed that the tests defined in "scripts" in composer.json are largely broken. For example, the path to CommonMarkTestStrict.php and CommonMarkTestWeak.php are incorrect (they should be prefixed with "vendor/erusev/parsedown/"), and even then many of the test scenarios are failing. This is not due to the changes in my PR, though - it looks like some general cleanup is needed in this repo to get it healthy again.

@davidbyoung
Copy link
Author

I've pushed an update to fix some of those issues, but likely more are needed. For example, we should probably add PHP 8.4 to CI and allow it to fail (it will until things like Psalm and PHP-CS-Fixer natively support PHP 8.4 without having to use things like PHP_CS_FIXER_IGNORE_ENV=1 to work around their lack of support).

@erusev
Copy link
Owner

erusev commented Sep 29, 2024

Do you know why we have 3 branches?

CleanShot 2024-09-29 at 17 39 17@2x

What's each branch for?

@davidbyoung
Copy link
Author

I'm not sure. It looked like 2.0.x was more up to date than master (eg it had GitHub Actions instead of TravisCI, it separated code into the src directory), which is why I rebased off it for this pull request.

@erusev
Copy link
Owner

erusev commented Sep 29, 2024

Hm, ok, but if we're making a release for these changes, shouldn't it be a 0.8.x release?

@davidbyoung
Copy link
Author

What's v2 for, then? Its most recent release was a few years after 0.8.x.

@erusev
Copy link
Owner

erusev commented Sep 29, 2024

It's probably a version of the extensions that was designed to work with Parsedown 2.0. It's something @aidantwoods was working on but it might be abandoned.

I'm also not sure why 0.8 is a separate branch from master.

There's a discussion about the same thing in Parsedown at erusev/parsedown#881.

@erusev
Copy link
Owner

erusev commented Oct 12, 2024

@aidantwoods can I merge 0.8 into master and delete it?

@aidantwoods
Copy link
Collaborator

It's been a while, but I think parsedown 1.8.x (and extra 0.8) had some risk of breaking extensions due to internal changes to escaping HTML. I think I put these out as prereleases because I wasn't sure what the BC promise should be for not breaking extensions (given they hook into a protected API surface rather than a public one).

@erusev
Copy link
Owner

erusev commented Oct 12, 2024

Do you think it still makes sense to have that branch? The latest release, 0.8.1 is almost 5 years old now.

We just merged the 1.8 of Parsedown into main here: erusev/parsedown#881

@aidantwoods
Copy link
Collaborator

If we're happy releasing 1.8 as stable I think there's no need to keep the separate branches around

@erusev
Copy link
Owner

erusev commented Oct 12, 2024

@davidbyoung

Can you create a PR that fixes PHP 8.4 deprecations in 0.8.x branch and also adds a GitHub workflow for testing similar to the one in the 2.0.x branch? I can then merge 0.8.x into main.

You can also use the GitHub workflow at https://github.com/erusev/parsedown as reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 8.4 deprecations
3 participants